-
Notifications
You must be signed in to change notification settings - Fork 214
Allow for vendor extensions via x-publisher #298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com>
Asked Claude to suggest how we can split it eventually -
|
Claude encountered an error —— View job
I'll analyze this and get back to you. |
@@ -1,34 +1,45 @@ | |||
# Server JSON Examples | |||
|
|||
_These examples show the PublishRequest format used by the `/v0/publish` API endpoint. Each example includes the `server` specification and optional `x-publisher` extensions for build metadata._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to clarify what exactly counts as server.json: I think it's just the bit under server
.
And this broader thing, we don't quite have a name for it (registry.json
was suggested here although I'm kinda lukewarm about that name).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good catch 👍 I agree with @tadasant that we should keep the server as it is and have a wrapper or a separate file. A wrapper like the registry(-entry).json is probably better, we can have a registry schema that describes it and reuse/reference the existing one for the server.
I'll fix the docs in a follow up to handle only the server and I can re-introduce these examples once we agree on the format for the whole entry 👍
@@ -15,7 +16,7 @@ import ( | |||
// PublishServerInput represents the input for publishing a server | |||
type PublishServerInput struct { | |||
Authorization string `header:"Authorization" doc:"Registry JWT token (obtained from /v0/auth/token/github)" required:"true"` | |||
Body model.PublishRequest | |||
RawBody []byte `body:"raw"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused why we do this - it seems like it could still remain some model, just that we verify that model has no extra keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right 👍 I've cleared a bunch of things like that as initially I approached it with a more looser format (in case we allow for trusted vendor extensions like "x-com.anthropic"), but it seems I've missed this. I'll fix it in the follow ups 👍
"strings" | ||
"time" | ||
|
||
"github.com/modelcontextprotocol/registry/internal/model" | ||
) | ||
|
||
// ReadSeedFile reads seed data from various sources: | ||
// 1. Local file paths (*.json files) | ||
// 2. Direct HTTP URLs to seed.json files | ||
// 1. Local file paths (*.json files) - expects extension wrapper format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe call this 'ServerRecord' format, so the naming across comments and models etc. is aligned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, will fix it as part of the follow ups 👍
-- This migration implements the extension wrapper architecture | ||
|
||
-- Create server_extensions table for registry/publisher metadata | ||
CREATE TABLE server_extensions ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I think we should probably just keep this in the one table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw I think this is probably my highest priority comment, but even this isn't very high priority.
if it's painful to edit the tables nicely with schema migrations, I'm pretty fine for you to rewrite migration 001 instead of creating migration 002 given we're not properly deployed yet, and I can just wipe the database and reapply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning is I thought it made sense to keep them in separate tables because it makes it easier to track the server.json as its own standalone object in one place while keeping all the extensions organised in another. This way we can look at each independently without things getting too tangled. That might be too much of course, so happy to keep it simple and have them in a single table for now (we can always split/migrate later) 👍
@@ -0,0 +1,170 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think once we have the updated files, we can probably discard this tool? Unless you forsee it being useful in other ways?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen a few discussions where others have stood up compatible registries, so I thought we can have it for some time as it may be useful for them too. If that's not the case I can do a follow up and remove it straight away or we can leave it for a few days and remove it before the go-live? In any case I have the same intention to discard it 😃
My opinion here:
@rdimitrov can you let me know if you'd object to me merging this now? |
data/seed-legacy-backup.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe other somewhat high priority comment would be: we should just delete this file, no need to hang around. git history keeps it anyway for us :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I'll file a follow up 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above
lmk if you object to a merge!
I think this is reasonable 🙏 I'm more than happy to get this merged now and do follow ups addressing your comments 👍 Thank you for reviewing it so promptly! |
Co-authored-by: adam jones <domdomegg+git@gmail.com>
This PR adds a new example to the OpenAPI specification that demonstrates the publisher extensions feature recently added in #298. ## Changes - Added \`example_org_server\` example to the OpenAPI spec - Demonstrates \`x-publisher\` fields for contact email and build metadata - Shows organization-specific extensions (\`x-com.example\`) including: - Marketplace icon URL - Category classification - Documentation URL - Verified publisher status This example helps developers understand how to use the new vendor extensions functionality when publishing servers to the registry. Co-authored-by: adam jones <adamj+git@anthropic.com>
This PR serves as a placeholder and brings together all of the changes related to supporting vendor extensions passed through the API. The idea is to have a single place to track and review the work as it comes together (even if it’s still a bit of a work in progress).
Disclaimer:
As I mentioned in Discord, this turned out to be a bit larger than I initially expected. The goal of this PR is to get it opened up early so we can kick off the first round of reviews while I continue refining and adding to it. That way, feedback can start flowing in parallel and help guide the remaining work.
The changes in this PR cover:
ServerResponse
-What's left
Motivation and Context
Motivated by the discussions in: #284
How Has This Been Tested?
Locally and by running all of the tests
Breaking Changes
Yes, it changes both the API and the format of the seed file
Types of changes
Checklist
Additional context